Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explicitly cancel blocked requests (attempt 2) #7030

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Explicitly cancel blocked requests (attempt 2) #7030

merged 4 commits into from
Nov 9, 2020

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Nov 3, 2020 •
edited by antonok-edm
Loading

Manual re-opening of #6632

This was merged and then reverted with #7028
Fixes brave/brave-browser#10063

This reverts commit 12ea1cd, reversing
changes made to fbce153.

Submitter Checklist:

Test Plan:

The below pages have all been observed to crash with OOM errors about 20 seconds after loading without this change, but should not crash once this change is in place:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton bsclifton requested a review from iefremov as a code owner November 3, 2020 04:45
@bsclifton bsclifton self-assigned this Nov 3, 2020
@bsclifton @antonok-edm
Revert "Merge pull request #7028 from brave/bsc-revert-6632"
f997dbf 
This reverts commit 12ea1cd, reversing
changes made to fbce153.
antonok-edm
antonok-edm approved these changes Nov 3, 2020
View reviewed changes
Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also updated the adblocking tests to use the more modern EvalJs instead of ExecuteScriptAndExtractBool, which made it a little easier to debug.

browser/net/brave_proxying_url_loader_factory.cc Outdated
if (ctx_->cancel_request_explicitly) {
OnRequestError(network::URLLoaderCompletionStatus(net::ERR_ABORTED));
if (!ctx_->ShouldMockRequest()) {
OnRequestError(network::URLLoaderCompletionStatus(net::ERR_BLOCKED_BY_CLIENT));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsclifton I just had to change net::ERR_ABORTED to net::ERR_BLOCKED_BY_CLIENT. The 4 test failures were occurring because the XMLHttpRequests were having their onabort handlers triggered instead of onerror.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 😄👍

@antonok-edm
fix failing tests
99b0cf8 
The XHRs from the 4 failing tests were triggering `onabort`, not
`onerror`.
@antonok-edm
modernize adblock tests
f157181
@antonok-edm
Copy link
Collaborator

These tests had to be updated as well, since they also use the blocking.html test page:

PerfPredictorTabHelperTest.NoBlockNoSavings
PerfPredictorTabHelperTest.ScriptBlockHasSavings
PerfPredictorTabHelperTest.NewNavigationStoresSavings

@bsclifton
Copy link
Member Author

cc: @iefremov for re-review 😄

Was originally approved in PR by @spinda but full CI didn't run since it was in a fork

@iefremov
Copy link
Contributor

iefremov commented Nov 4, 2020

Thanks for fixing the tests!

@bsclifton I just had to change net::ERR_ABORTED to net::ERR_BLOCKED_BY_CLIENT. The 4 test failures were occurring because the XMLHttpRequests were having their onabort handlers triggered instead of onerror.

Wondering, does this change (from onabort to onerror) bring any potential webcompat risks?

@antonok-edm
Copy link
Collaborator

Wondering, does this change (from onabort to onerror) bring any potential webcompat risks?

@iefremov webcompat was the motivating reason behind this change actually. Adblocking extensions only have the ability to do net::ERR_BLOCKED_BY_CLIENT, so they design their adblock lists and scriptlets accordingly. We've run into a handful of issues where the difference makes us less compatible, given that we're using the same rules as uBlock Origin.

@bsclifton bsclifton mentioned this pull request Nov 4, 2020
Closed
@antonok-edm
Copy link
Collaborator

@bsclifton anything left blocking this, or can we merge?

@antonok-edm
Copy link
Collaborator

CI failures are known, merging.

@antonok-edm antonok-edm merged commit 9719465 into master Nov 9, 2020
@antonok-edm antonok-edm deleted the bsc-reland-6632 branch November 9, 2020 17:28
@antonok-edm antonok-edm added this to the 1.18.x - Nightly milestone Nov 9, 2020
@brave-builds brave-builds mentioned this pull request Nov 9, 2020
Merged
4 tasks
@iefremov iefremov restored the bsc-reland-6632 branch November 10, 2020 06:18
iefremov pushed a commit that referenced this pull request Nov 10, 2020
@brave-browser-releases @iefremov
Uplift of #7030 (squashed) to release
e4717fc
@iefremov iefremov mentioned this pull request Nov 10, 2020
Merged
4 tasks
@antonok-edm antonok-edm deleted the bsc-reland-6632 branch November 10, 2020 15:14
@kjozwiak
Copy link
Member

kjozwiak commented Nov 10, 2020 •
edited
Loading

Verification PASSED on macOS 10.15.7 x64 using the following build:

Brave | 1.18.46 Chromium: 87.0.4280.49 (Official Build) unknown (x86_64)
-- | --
Revision | f77f85899646b42a1d3c8ff36794e00becab9171-refs/branch-heads/4280@{#1115}
OS | macOS Version 10.15.7 (Build 19H2)

Verification PASSED on Samsung S10+ on Android 10 using 1.18.46 CR: 87.0.4280.49

SergeyZhukovsky pushed a commit that referenced this pull request Nov 10, 2020
@brave-browser-releases @SergeyZhukovsky
Uplift of #7030 (squashed) to beta
6f35215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iefremov iefremov iefremov approved these changes

@antonok-edm antonok-edm antonok-edm approved these changes

@pes10k pes10k Awaiting requested review from pes10k

Assignees

@bsclifton bsclifton

Labels
None yet
Projects
None yet
Milestone
1.18.x - Release
4 participants
@bsclifton @antonok-edm @iefremov @kjozwiak